feat(mcms): add inspectors and converters helpers for mcms chain client loading #667
Conversation
🦋 Changeset detectedLatest commit: 0d71bda The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| converter = &evm.TimelockConverter{} | ||
| case chainsel.FamilySolana: | ||
| converter = solana.TimelockConverter{} | ||
| case chainsel.FamilyAptos: |
There was a problem hiding this comment.
aren't we missing a few chains here? Like Sui?
gustavogama-cll
left a comment
gustavogama-cll
left a comment
There was a problem hiding this comment.
So, I love that you're trying to reduce the duplication we have right now but I feels we could give this a bit more thought. In particular, I think most of these function (maybe all) could go in the mcms library itself, no CLDF's experimental package...
I see the inspectors part could be a bit more challenge as there's a dependency on CLDF's Blockchains object, but maybe that's precisely what we need to figure out: how to define an interface in the mcms library that is implemented by cldf's Blockchains.
If you like the idea and are willing to wait a couple of hours, I can try to run a few local tests to see how that would look like.
Yeah I'm all for it, having it mcmslib would be great. Sure lets explore this one a bit more, I think it's worth taking a bit more time to fix these things the proper way. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var converter sdk.TimelockConverter | ||
| switch fam { | ||
| case chainsel.FamilyEVM: | ||
| converter = &evm.TimelockConverter{} |
There was a problem hiding this comment.
Inconsistent converter instantiation: EVM uses pointer while Solana (line 29) uses value type. Consider making the pattern consistent across all chain families.
|
Here's my attempt at implementing this in mcms lib. There's a comment about a change we'd need in CLDF. |
|
nice looking good! I'll close this one, I was trying a similar approach smartcontractkit/mcms#579 which was a lot of similarities to yours |
2 participants
We are moving some of the helpers to load inspectors and converters from mcms-tools since these functions are also used in some of the composite actions in CLD and we don't want to inject new repo dependencies. This change will allow us to load inspectors for getting on chain opcounts in tools we are building to update all domain opcounts in proposals. Not doing this would require to have copies of these functions which are chain family dependant, making it harder to add new chain families. Thus, I opted to move it to a single place where it can easily be imported as a library to keep new chain integrations more streamlined.
AI Summary
This pull request introduces new helpers for working with MCMS chain client loaders, specifically adding inspector and converter utilities to the
chainlink-deployments-framework. These helpers make it easier to build and fetch chain-specific inspectors and converters for timelock proposals, and include robust unit tests for the new logic. Additionally, the PR updates configuration and dependency files to support these additions.The most important changes are:
Inspector and Converter Helpers:
inspectors.goandconverters.goinexperimental/proposalutilsto provide generic helpers for building and fetching chain-specific inspectors and converters for MCMS timelock proposals. These helpers support EVM, Solana, and Aptos chains, and handle chain selector family detection. [1] [2]InspectorFetcherinterface and its implementation,MCMInspectorFetcher, for fetching inspectors based on chain metadata and action.Chain Metadata Utilities:
aptos_helpers.goandsui_helpers.goinexperimental/proposalutils/chainsmetadatato provide utility functions for extracting roles and metadata from MCMS timelock proposals for Aptos and Sui chains. [1] [2]Testing:
inspectors_test.go,converters.go(indirectly tested via inspectors),chainsmetadata/aptos_helpers_test.go, andchainsmetadata/sui_helpers_test.go. [1] [2] [3]Mocks and Configuration:
MockInspectorFetcherfor testing, and updated.mockery.ymlto generate mocks for the new interface. [1] [2]Changelog and Dependency:
go.modto explicitly requiregithub.com/samber/lo.